Skip to content

fix query params sorting to build correct canonical query string #1938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jderusse merged 6 commits into from
Sep 4, 2025
Merged

fix query params sorting to build correct canonical query string #1938

jderusse merged 6 commits into from
Sep 4, 2025

Conversation

@DmytroVyshnevsky1
Copy link
Contributor

@DmytroVyshnevsky1 DmytroVyshnevsky1 commented Sep 2, 2025 •
edited
Loading

Hello!

I found a bug while working with Async AWS , AWS sdk have the same one.

If one of the query params is an array with 10+ elements it causes an error:
"The request signature we calculated does not match the signature you provided".
The root of problem is sorting of query params for canonical string, cause params should be sorted by name AFTER url encoding.
You can read about this here

quote:
" You must also sort the parameters in the canonical query string alphabetically by key name. The sorting occurs after encoding."
This ksort is the reason of problem.

Example URL:

'https://example.com/service?transaction_ids[0]=1111&transaction_ids[1]=1111&transaction_ids[10]=1111&transaction_ids[2]=1111&transaction_ids[3]=1111&transaction_ids[4]=1111&transaction_ids[5]=1111&transaction_ids[6]=1111&transaction_ids[7]=1111&transaction_ids[8]=1111&transaction_ids[9]=1111'

The example of sorting result on current version:

array(11) {
  ["transaction_ids[0]"]=>
  string(4) "1111"
  ["transaction_ids[10]"]=>
  string(4) "1111"
  ["transaction_ids[1]"]=>
  string(4) "1111"
  ["transaction_ids[2]"]=>
  string(4) "1111"
  ["transaction_ids[3]"]=>
  string(4) "1111"
  ["transaction_ids[4]"]=>
  string(4) "1111"
  ["transaction_ids[5]"]=>
  string(4) "1111"
  ["transaction_ids[6]"]=>
  string(4) "1111"
  ["transaction_ids[7]"]=>
  string(4) "1111"
  ["transaction_ids[8]"]=>
  string(4) "1111"
  ["transaction_ids[9]"]=>
  string(4) "1111"
}

and the correct one after this fix:

array(11) {
  ["transaction_ids[0]"]=>
  string(7) "1111"
  ["transaction_ids[1]"]=>
  string(7) "1111"
  ["transaction_ids[10]"]=>
  string(7) "1111"
  ["transaction_ids[2]"]=>
  string(7) "1111"
  ["transaction_ids[3]"]=>
  string(7) "1111"
  ["transaction_ids[4]"]=>
  string(7) "1111"
  ["transaction_ids[5]"]=>
  string(7) "1111"
  ["transaction_ids[6]"]=>
  string(7) "1111"
  ["transaction_ids[7]"]=>
  string(7) "1111"
  ["transaction_ids[8]"]=>
  string(7) "1111"
  ["transaction_ids[9]"]=>
  string(7) "1111"
}

As you can see current sorting put 10 and 1 in wrong places. This not allowed signer to build correct canonical query string.
Also need to clarify that Query parameters are added to the request via:

AsyncAws\Core\Request::setQueryAttribute

@DmytroVyshnevsky1 DmytroVyshnevsky1 force-pushed the fix-query-params-sorting-for-SigV4 branch from 87c2dd2 to b0154ba Compare September 2, 2025 22:54
jderusse
jderusse reviewed Sep 3, 2025
View reviewed changes
Copy link
Member

@jderusse jderusse left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the investigation end the psycho pull request.
could you please provide a test for this ?

@DmytroVyshnevsky1
Copy link
Contributor Author

DmytroVyshnevsky1 commented Sep 3, 2025 •
edited by jderusse
Loading

@jderusse
Thanks for the quick review — I can add tests. Also, could you clarify what you meant by “end the psycho”? Was that a typo?

@jderusse
Copy link
Member

jderusse commented Sep 3, 2025

@jderusse Thanks for the quick review — I can add tests. Also, could you clarify what you meant by “end the psycho”? Was that a typo?

uh.. my keyboard replaced pull reaquest by psycho 😅

@DmytroVyshnevsky1
Copy link
Contributor Author

DmytroVyshnevsky1 commented Sep 4, 2025

@jderusse
I added the tests,
I think they also will help if someone will try to add proper parsing for query string with arrays

@jderusse
Copy link
Member

jderusse commented Sep 4, 2025

thank you @DmytroVyshnevsky1

@jderusse jderusse merged commit 87fd89e into async-aws:master Sep 4, 2025
17 checks passed
This was referenced Oct 23, 2025
Closed
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jderusse jderusse jderusse left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DmytroVyshnevsky1 @jderusse